fix: don't deploy metadata files to CDN #2104
Conversation
✅ Deploy Preview for netlify-plugin-nextjs-nx-monorepo-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-static-root-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-next-auth-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-export-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for nextjs-plugin-custom-routes-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-plugin-edge-middleware ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-plugin-canary ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-i18next-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| '/react-loadable-manifest.json', | ||
| '/BUILD_ID', | ||
| ] | ||
| export const HIDDEN_PATHS = destr(process.env.NEXT_KEEP_METADATA_FILES) |
There was a problem hiding this comment.
this is primarily for test/e2e as those try to check metadata files after deployment, demos + cypress tests will delete those files on the other hand
There was a problem hiding this comment.
Since it's for E2E/CI only (primarily), we already have a CI environment variable. See a usage of it here, https://github.com/netlify/next-runtime/blob/82c1ea371bbe2194058264d7cfad912343334809/package.json#L19.
| export const HIDDEN_PATHS = destr(process.env.NEXT_KEEP_METADATA_FILES) | |
| export const HIDDEN_PATHS = destr(process.env.CI) |
There was a problem hiding this comment.
CI env var is true on Netlify platform, so we can't rely on that, otherwise we won't actually delete those meta files :(
| '/react-loadable-manifest.json', | ||
| '/BUILD_ID', | ||
| ] | ||
| export const HIDDEN_PATHS = destr(process.env.NEXT_KEEP_METADATA_FILES) |
There was a problem hiding this comment.
Since it's for E2E/CI only (primarily), we already have a CI environment variable. See a usage of it here, https://github.com/netlify/next-runtime/blob/82c1ea371bbe2194058264d7cfad912343334809/package.json#L19.
| export const HIDDEN_PATHS = destr(process.env.NEXT_KEEP_METADATA_FILES) | |
| export const HIDDEN_PATHS = destr(process.env.CI) |
| @@ -467,3 +478,10 @@ export const movePublicFiles = async ({ | |||
| await copy(publicDir, `${publish}${basePath}/`) | |||
| } | |||
| } | |||
|
|
|||
| export const removeMetadataFiles = async (publish: string) => { | |||
There was a problem hiding this comment.
Would it not be faster to await Promise.all() or await Promise.allSettled()?
There was a problem hiding this comment.
Generally yes, but if we don't want to do 1 by 1 for perf reasons we should still limit concurrency as otherwise some starvation might happen and doing just Promise.all() might actually be slower than doing it 1 by 1
For files moving we do have concurency of number of cpu cores, with minimum of 2 (if on single core):
So I would suggest we do the same here?
There was a problem hiding this comment.
Swapped to p-limit (like snippet above) in d709eb1
There was a problem hiding this comment.
Thanks for this @pieh. The env var for preserving metadata files for E2E tests is not a blocker. I'll let you decide if you opt for the existing CI env var or not. 🚢
Summary
Current strategy of hiding metadata files (creating redirects to 404) has a flaw - redirects are case sensitive, but CDN is not so that means we can still actually reach those files by messing with route casing.
As example
/traceis supposed to be hidden - and initially it appears to be: https://netlify-plugin-nextjs-demo.netlify.app/trace, however https://netlify-plugin-nextjs-demo.netlify.app/tRace allow to view thetracefile. Preview Deploy for this PR actually make it impossible to serve that asset regardless of casing as it's just not deployed to CDN: https://deploy-preview-2104--netlify-plugin-nextjs-demo.netlify.app/tRaCe/This PR changes "hiding metadata files" implementation to actually prevent those files from being deployed by deleting them after all the functions bundling (that rely on those metadata files) is finished.
This change also required adjustment to 404 documents - we were not moving them from
.next/server(less)/pagesbefore - now they are moved the same as regular html files and 404 redirects were updated to match it. This was needed because we do delete.next/server(less)before deployment so those 404 documents were just lost and not available to be served.Test plan
Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal
Standard checks:
🧪 Once merged, make sure to update the version if needed and that it was published correctly.
Ref: https://github.com/netlify/pod-ecosystem-frameworks/issues/491